follow-up fix for PR744 #745

flohump · 2024-11-12T08:13:25Z

🐦 Description of this PR 🐦

this PR resolves the following issues:
a) land area is not constant over time
b) infeasibilites in 44_biodiversity module in FSEC runs

10_land Simplified land transition matrix for improved feasibility (removed balance variables!)
modules[29-35] added initial vales for ov_bv for better starting point
44_biodiversity bugfix biorealm_biome.cs3 in input data (shares did not add-up to 1)

🔧 Checklist for PR creator 🔧

Label pull request from the label list.
- Low risk: Simple bugfixes (missing files, updated documentation, typos) or changes in start or output scripts
- Medium risk: Uncritical changes in the model core (e.g. moderate modifications in non-default realizations)
- High risk: Critical changes in model core or default settings (e.g. changing a model default or adjusting a core mechanic in the model)
Self-review own code
- No hard coded numbers and cluster/country/region names.
- The new code doesn't contain declared but unused parameters or variables.
- magpie4 R library has been updated accordingly and backwards compatible where necessary.
- scenario_config.csv has been updated accordingly (important if default.cfg has been updated)
Document changes
- Add changes to CHANGELOG.md
- Where relevant, put In-code documentation comments
- Properly address updates in interfaces in the module documentations
- run goxygen::goxygen() and verify the modified code is properly documented
Perform test runs
- Low risk:
  - Run a compilation check via Rscript start.R --> "compilation check"
- Medium risk:
  - Run test runs via Rscript start.R --> "test runs"
  - Check logs for errors/warnings
- High risk:
  - Run test runs via Rscript start.R --> "test runs"
  - Check logs for errors/warnings
  - Default run from the PR target branch for comparison
  - Provide relevant comparison plots (land-use, emissions, food prices, land-use intensity,...)

All default test runs including 4 FSEC runs (test_runs.R) are feasible.

📉 Performance changes 📈

Current develop branch default : 25 mins
This PR's default : 25 mins

🚨 Checklist for reviewer 🚨

PR is labeled correctly
Code changes look reasonable
- No hard coded numbers and cluster/country/region names.
- No unnecessary increase in module interfaces
- model behavior/performance is satisfactory.
Changes are properly documented
- CHANGELOG is updated correctly
- Updates in interfaces have been properly addressed in the module documentations
- In-code documentation looks appropriate
content review done (at least 1)
RSE review done (at least 1)

k4rst3ns · 2024-11-12T10:01:00Z

modules/10_land/landmatrix_dec18/equations.gms


 q10_transition_to(j2,land_to) ..
  sum(land_from, vm_lu_transitions(j2,land_from,land_to)) =e=
  vm_land(j2,land_to);

 q10_transition_from(j2,land_from) ..
  sum(land_to, vm_lu_transitions(j2,land_from,land_to)) =e=
-  vm_land.l(j2,land_from) + v10_balance_positive(j2,land_from) - v10_balance_negative(j2,land_from);
+  pcm_land(j2,land_from);


why not vm_land.l? I thought this was one of the points of PR before to have here same accuracy.

It turned out that vm_land.l and pcm_land differ considerably.
It seems that vm_land.l is not updated in all realizations.

pascal-sauer

Looks good, thanks!

config/default.cfg

pascal-sauer · 2024-11-13T12:51:27Z

modules/44_biodiversity/bii_target/sets.gms

@@ -30,3 +30,5 @@ sets
    OC1,OC2,OC7,NT3,NT4,NT7,NT8,NT10,NT12,NT13,NT14,PA11,PA12,PA13,NA5,PA1,PA4,PA5,PA6,
    PA8,PA9,PA10,AN99,AT98,NA2,NA6,NA7,NA12,NA99,PA98,PA99,AA12,AA13 /  
 ;
+
+alias(biome44,biome44_2);


Why is this alias needed?

This is needed for the calculation of i44_biome_share(j,biome44)

i44_biome_share(j,biome44) = (f44_biome_area(j,biome44) + 1e-10) / sum(biome44_2, f44_biome_area(j,biome44_2) + 1e-10);

Out of curiosity, why does it not work to just use biome44 instead of biome44_2 everywhere?

Because the set biome44 is already under control. It's just not possible in GAMS to do this operation with the same set.

Ah, under control means it appears on the left hand side of the equation (or is it an assignment?)?

Under control in GAMS means that all calculations are done for each set element.

# Conflicts: # main.gms

pvjeetze

Thanks a lot for these extensive and fast updates!

I requested some minor changes.

In addition we should have again a discussion and a decision in the MAgPIE meeting on how to deal with project-specific scenario configs.

CHANGELOG.md

pvjeetze · 2024-11-14T07:49:37Z

config/projects/scenario_config_el2.csv

@@ -19,5 +19,5 @@ gms$s15_exo_alcohol;1;1;1
 gms$s15_alc_scen;0;0;0
 gms$factor_costs;sticky_labor;sticky_labor;sticky_labor
 gms$c70_feed_scen;ssp1;ssp2;ssp2
-input['cellular'];rev4.115EL2_h12_c6a7458f_cellularmagpie_c200_IPSL-CM6A-LR-ssp370_lpjml-8e6c5eb1.tgz;;
+input['cellular'];rev4.116EL2_h12_c6a7458f_cellularmagpie_c200_IPSL-CM6A-LR-ssp370_lpjml-8e6c5eb1.tgz;;


Not entirely sure, whether we'd come up with a final decision on this, but I thought that it is now in the responsibility of the people taking care of the respective projects to keep the project-specific scenario_configs up to date. Similar as with the project related start scripts?
Maybe we could have a dedicated discussion on this again in the MAgPIE meeting.

Thanks for bringing this up. Let's discuss about this in the MAgPIE meeting.

pvjeetze · 2024-11-14T07:54:15Z

config/projects/scenario_config_fsec.csv

The FSEC scenario config maybe a separate case, or maybe even not (see comment above). One idea of splitting the project specific settings from the main scenario_config was that for general developments only the main scenario_config must be updated and thereby taking away the burden of also maintaining project-specific ones.

Since we use FSEC in our default test runs, it should be updated from my view.

modules/44_biodiversity/bii_target/equations.gms

modules/44_biodiversity/bii_target/preloop.gms

modules/44_biodiversity/bii_target/presolve.gms

modules/44_biodiversity/bii_target_apr24/preloop.gms

modules/44_biodiversity/bii_target_apr24/presolve.gms

Co-authored-by: pvjeetze <[email protected]>

pvjeetze

Thanks for the updates. Looks good for me now.

I only have a minor addtional comment regarding the placement of code in the preloop.

modules/44_biodiversity/bii_target/preloop.gms

modules/44_biodiversity/bii_target_apr24/preloop.gms

flohump added 6 commits November 11, 2024 18:45

bugfix vm_land

6565d7a

test

f817640

test

84bfeee

test

a173ead

test

6edaf63

revert

1e3f522

k4rst3ns reviewed Nov 12, 2024

View reviewed changes

flohump added 6 commits November 12, 2024 21:30

update

8e865af

Merge branch 'develop' of github.com:magpiemodel/magpie into f_fixOpti

d9af1da

fix

a3ff847

update

9e0c3bd

bugfixes

5eb9664

bugfix

6cc883e

flohump requested review from pvjeetze, pascal-sauer and FelicitasBeier November 13, 2024 10:10

flohump added High risk Higher risk bugfix labels Nov 13, 2024

flohump marked this pull request as ready for review November 13, 2024 10:13

codeCheck update

172f8c3

pascal-sauer approved these changes Nov 13, 2024

View reviewed changes

flohump added 3 commits November 13, 2024 16:58

removed neg scaling factors

1f39058

clean-up scaling factors

3a4ddd8

Merge branch 'develop' of github.com:magpiemodel/magpie into f_fixOpti

77cb4d5

# Conflicts: # main.gms

pvjeetze requested changes Nov 14, 2024

View reviewed changes

flohump and others added 5 commits November 14, 2024 09:32

Update CHANGELOG.md

849c9a0

Co-authored-by: pvjeetze <[email protected]>

Update modules/44_biodiversity/bii_target/equations.gms

6069d07

Co-authored-by: pvjeetze <[email protected]>

Update modules/44_biodiversity/bii_target/preloop.gms

9d89010

Co-authored-by: pvjeetze <[email protected]>

Update modules/44_biodiversity/bii_target_apr24/preloop.gms

ad9bc11

Co-authored-by: pvjeetze <[email protected]>

remove i44_biome_area_reg from presolve

01af64c

flohump requested a review from pvjeetze November 14, 2024 08:41

pvjeetze approved these changes Nov 14, 2024

View reviewed changes

modules/44_biodiversity/bii_target/preloop.gms Outdated Show resolved Hide resolved

modules/44_biodiversity/bii_target_apr24/preloop.gms Outdated Show resolved Hide resolved

flohump added 3 commits November 14, 2024 09:57

removed duplicate code

3888668

update start script

6642735

magpie4 lib

3bb99fc

flohump merged commit 408e5a8 into magpiemodel:develop Nov 15, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

follow-up fix for PR744 #745

follow-up fix for PR744 #745

flohump commented Nov 12, 2024 •

edited

Loading

k4rst3ns Nov 12, 2024

flohump Nov 13, 2024

pascal-sauer left a comment

pascal-sauer Nov 13, 2024

flohump Nov 13, 2024

pascal-sauer Nov 13, 2024

flohump Nov 13, 2024

pascal-sauer Nov 13, 2024 •

edited

Loading

flohump Nov 14, 2024

pvjeetze left a comment

pvjeetze Nov 14, 2024

flohump Nov 14, 2024

pvjeetze Nov 14, 2024

flohump Nov 14, 2024

pvjeetze left a comment

follow-up fix for PR744 #745

follow-up fix for PR744 #745

Conversation

flohump commented Nov 12, 2024 • edited Loading

🐦 Description of this PR 🐦

🔧 Checklist for PR creator 🔧

📉 Performance changes 📈

🚨 Checklist for reviewer 🚨

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascal-sauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascal-sauer Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvjeetze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvjeetze left a comment

Choose a reason for hiding this comment

flohump commented Nov 12, 2024 •

edited

Loading

pascal-sauer Nov 13, 2024 •

edited

Loading